Skip to content

[Support] Do not remove lock file on failure #130834

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 12, 2025

Conversation

jansvoboda11
Copy link
Contributor

Clients of LockFileManager call unsafeRemoveLockFile() whenever tryLock() fails. However looking at the code, there are no scenarios where this actually does something useful. This PR removes such calls.

@llvmbot llvmbot added clang Clang issues not falling into any other category backend:AMDGPU llvm:support labels Mar 11, 2025
@jansvoboda11
Copy link
Contributor Author

I added annotations to LockFileManager.cpp that explain why I think calling unsafeRemoveLockFile() (which just removes LockFileName) does not have any effect. I will remove those annotations before merging.

@llvmbot
Copy link
Member

llvmbot commented Mar 11, 2025

@llvm/pr-subscribers-backend-amdgpu
@llvm/pr-subscribers-llvm-support

@llvm/pr-subscribers-clang

Author: Jan Svoboda (jansvoboda11)

Changes

Clients of LockFileManager call unsafeRemoveLockFile() whenever tryLock() fails. However looking at the code, there are no scenarios where this actually does something useful. This PR removes such calls.


Full diff: https://github.com/llvm/llvm-project/pull/130834.diff

3 Files Affected:

  • (modified) clang/lib/Frontend/CompilerInstance.cpp (-2)
  • (modified) llvm/lib/Support/LockFileManager.cpp (+8-6)
  • (modified) llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp (-1)
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index e9e96683ca51f..44f4f48ef94e8 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -1491,8 +1491,6 @@ static bool compileModuleAndReadASTBehindLock(
       // related errors.
       Diags.Report(ModuleNameLoc, diag::remark_module_lock_failure)
           << Module->Name << toString(std::move(Err));
-      // Clear out any potential leftover.
-      Lock.unsafeRemoveLockFile();
       return compileModuleAndReadASTImpl(ImportingInstance, ImportLoc,
                                          ModuleNameLoc, Module, ModuleFileName);
     }
diff --git a/llvm/lib/Support/LockFileManager.cpp b/llvm/lib/Support/LockFileManager.cpp
index 7cf9db379974f..cbffd11e547bd 100644
--- a/llvm/lib/Support/LockFileManager.cpp
+++ b/llvm/lib/Support/LockFileManager.cpp
@@ -169,7 +169,7 @@ Expected<bool> LockFileManager::tryLock() {
   SmallString<128> AbsoluteFileName(FileName);
   if (std::error_code EC = sys::fs::make_absolute(AbsoluteFileName))
     return createStringError(EC, "failed to obtain absolute path for " +
-                                     AbsoluteFileName);
+                                     AbsoluteFileName); // We don't even know the LockFileName yet.
   LockFileName = AbsoluteFileName;
   LockFileName += ".lock";
 
@@ -180,6 +180,8 @@ Expected<bool> LockFileManager::tryLock() {
     return false;
   }
 
+  // LockFileName either does not exist or we could not read it and removed it in readLockFile().
+
   // Create a lock file that is unique to this instance.
   UniqueLockFileName = LockFileName;
   UniqueLockFileName += "-%%%%%%%%";
@@ -187,13 +189,13 @@ Expected<bool> LockFileManager::tryLock() {
   if (std::error_code EC = sys::fs::createUniqueFile(
           UniqueLockFileName, UniqueLockFileID, UniqueLockFileName))
     return createStringError(EC, "failed to create unique file " +
-                                     UniqueLockFileName);
+                                     UniqueLockFileName); // LockFileName still does not exist.
 
   // Write our process ID to our unique lock file.
   {
     SmallString<256> HostID;
     if (auto EC = getHostID(HostID))
-      return createStringError(EC, "failed to get host id");
+      return createStringError(EC, "failed to get host id"); // LockFileName still does not exist. We may leave behind UniqueLockFileName though, this that in a follow-up.
 
     raw_fd_ostream Out(UniqueLockFileID, /*shouldClose=*/true);
     Out << HostID << ' ' << sys::Process::getProcessId();
@@ -207,7 +209,7 @@ Expected<bool> LockFileManager::tryLock() {
       sys::fs::remove(UniqueLockFileName);
       // Don't call report_fatal_error.
       Out.clear_error();
-      return std::move(Err);
+      return std::move(Err); // LockFileName still does not exist.
     }
   }
 
@@ -227,7 +229,7 @@ Expected<bool> LockFileManager::tryLock() {
 
     if (EC != errc::file_exists)
       return createStringError(EC, "failed to create link " + LockFileName +
-                                       " to " + UniqueLockFileName);
+                                       " to " + UniqueLockFileName); // We failed to create LockFileName for a weird reason.
 
     // Someone else managed to create the lock file first. Read the process ID
     // from the lock file.
@@ -248,7 +250,7 @@ Expected<bool> LockFileManager::tryLock() {
     // ownership.
     if ((EC = sys::fs::remove(LockFileName)))
       return createStringError(EC, "failed to remove lockfile " +
-                                       UniqueLockFileName);
+                                       UniqueLockFileName); // Failed to remove LockFileName. Why try to do it again?
   }
 }
 
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp b/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp
index 65cbb13628301..e2e449f1c8a38 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp
@@ -1552,7 +1552,6 @@ PreservedAnalyses AMDGPUSplitModulePass::run(Module &M,
         LLVM_DEBUG(
             dbgs() << "[amdgpu-split-module] unable to acquire lockfile, debug "
                       "output may be mangled by other processes\n");
-        Lock.unsafeRemoveLockFile();
       } else if (!Owned) {
         switch (Lock.waitForUnlock()) {
         case LockFileManager::Res_Success:

Copy link

github-actions bot commented Mar 11, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Contributor

@Bigcheese Bigcheese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reasoning makes sense. On error we either didn't actually create the lock file yet, or failed to remove it. It may be good to document tryLock with something about this.

Not directly related, but for UniqueLockFileName it would be nice if we could use O_TMPFILE on systems that support it.

@jansvoboda11 jansvoboda11 merged commit f4d599c into llvm:main Mar 12, 2025
11 checks passed
@jansvoboda11 jansvoboda11 deleted the lock-file-manager-remove branch March 12, 2025 15:51
frederik-h pushed a commit to frederik-h/llvm-project that referenced this pull request Mar 18, 2025
Clients of `LockFileManager` call `unsafeRemoveLockFile()` whenever
`tryLock()` fails. However looking at the code, there are no scenarios
where this actually does something useful. This PR removes such calls.
jansvoboda11 added a commit to swiftlang/llvm-project that referenced this pull request Mar 18, 2025
Clients of `LockFileManager` call `unsafeRemoveLockFile()` whenever
`tryLock()` fails. However looking at the code, there are no scenarios
where this actually does something useful. This PR removes such calls.

(cherry picked from commit f4d599c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:AMDGPU clang Clang issues not falling into any other category llvm:support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants